Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Logs UI] Add shared observability page template and navigation #99380

Merged
merged 31 commits into from
May 27, 2021

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented May 5, 2021

📝 Summary

This exposes a navigation registry and a shared page template on the observability setup() and start() contract respectively.

closes #98212

🎨 Previews

image

image

🕵️ Review notes

To cause the logs and metrics apps to show up in the sidebar, the following patch could be applied:

M x-pack/plugins/infra/public/plugin.ts
@@ -7,6 +7,7 @@
 
 import { i18n } from '@kbn/i18n';
 import { AppMountParameters, PluginInitializerContext } from 'kibana/public';
+import { of } from 'rxjs';
 import { DEFAULT_APP_CATEGORIES } from '../../../../src/core/public';
 import { createMetricThresholdAlertType } from './alerting/metric_threshold';
 import { createInventoryMetricAlertType } from './alerting/inventory';
@@ -48,6 +49,28 @@ export class Plugin implements InfraClientPluginClass {
         hasData: createMetricsHasData(core.getStartServices),
         fetchData: createMetricsFetchData(core.getStartServices),
       });
+
+      pluginsSetup.observability.navigation.registerSections(
+        of([
+          {
+            label: 'Logs',
+            sortKey: 200,
+            entries: [
+              { label: 'Stream', app: 'logs', path: '/stream' },
+              { label: 'Anomalies', app: 'logs', path: '/anomalies' },
+              { label: 'Categories', app: 'logs', path: '/categories' },
+            ],
+          },
+          {
+            label: 'Metrics',
+            sortKey: 300,
+            entries: [
+              { label: 'Inventory', app: 'metrics', path: '/inventory' },
+              { label: 'Metrics Explorer', app: 'metrics', path: '/explorer' },
+            ],
+          },
+        ])
+      );
     }
 
     pluginsSetup.embeddable.registerEmbeddableFactory(

Progress

  • create registry
    • takes observable (or object)
    • merges them to provide overall observable navigation structure
    • documented
    • tested
  • add registry as obs plugin instance variable
  • expose registration function via setup contract
  • create page template component
    • consumes observable navigation structure
    • renders page layout
    • renders side nav
    • documented
    • tested
  • expose page template via start contract
  • use in obs overview
    • obs overview registers itself via registration function
    • obs overview renders page template
  • use layout in alerts page
  • use layout in cases page
  • delete unused old layout code
  • ensure all relevant new strings are translated
  • check the bundle size increases
  • make sure the "add data" header portal is retained
  • use the stylized solution navigation sidebar ([KibanaPageLayout] Solution Nav specific styles & props #100089)

@weltenwort weltenwort added v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.14.0 labels May 5, 2021
@weltenwort weltenwort added this to the Logs UI 7.14 milestone May 5, 2021
@weltenwort weltenwort self-assigned this May 5, 2021
@snide
Copy link
Contributor

snide commented May 14, 2021

/oblt-deploy

@weltenwort
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@weltenwort weltenwort added release_note:enhancement Project:ObservabilitySolutionNavigation Unified Observability Work to make the observability apps more consistent and eventually more unified as a user experience labels May 26, 2021
@weltenwort
Copy link
Member Author

@katefarrar we'd appreciate a review from the design perspective as well

Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes all look good to me 🎉

bodyColor={theme.eui.euiPageBackgroundColor}
>
<CentralizedFlexGroup>
<ObservabilityPageTemplate template="centeredBody">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, because this works just fine, but the <KibanaPageTemplate /> accepts a isEmptyState prop, if set to true it will look at which content exists (children, header etc) and determine the correct template itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I wonder, though, if semantically the loading screen qualifies as an empty state.

@cchaos @katefarrar can you provide any guidance on how to best render full-page loading states with the <KibanaPageTemplate />? We're currently setting template="centeredBody" to display a centered spinner with accompanying loading text.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't anything that the Eui/KibanaPageTemplate provides specifically semantically to indicate empty state. It's purely a visual representation for consistency. Do you have a screenshot of your loading state that I could see for better advice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I noticed the same when I was testing it out. The original loading state sat within a small panel, but now with the new page component, I think it'd make more sense to remove the panel around the loading message and show the content area background? With this new layout, we're always expecting to have the main content area there.

Screenshot 2021-05-27 at 09 00 57

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks ok with just template="centeredContent". What do you think?

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weltenwort I wonder where the background is coming from around the spinner and text? If it's a panel, it supports color="transparent" to avoid it adding that light grey background.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no extra panel. The shaded background is also visible in the EUI examples:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems I can remove it using pageContentProps={{ color: 'transparent' }} on the template. Does it make sense to deviate from the EUI defaults, though?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, probably shouldn't go down that route. Let's leave as is, but I'll speak to @cchaos on how we want to handle full page loading states in the new layouts. Thanks for taking a look, and for fixing the original style issue 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked it over with @formgeist and I'll try to build something into the page template. Though it will most likely still follow the same pattern as the empty state, so I'd suggest using that for now until we have something specific for loading.

@weltenwort
Copy link
Member Author

/oblt-deploy

Copy link
Contributor

@formgeist formgeist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weltenwort Great job on getting the new navigation in - can't wait to get it into all of the apps, so you have the full experience. I noticed some minor things we probably should look into;

  • If an app is hidden in the Spaces settings, the navigation isn't hidden. The Kibana navigation item is, but the section and links still show, so we should make sure we respect the Space settings.

Screenshot 2021-05-27 at 08 51 51

Otherwise, I think this is good to go 👍

@weltenwort
Copy link
Member Author

Thanks for the review, @formgeist!

If an app is hidden in the Spaces settings, the navigation isn't hidden. The Kibana navigation item is, but the section and links still show, so we should make sure we respect the Space settings.

The patch to add these menu entries is just for demonstration purposes and not part of the PR. Since the side nav can't know about the relation between specific permissions and menu entries, it's up to the registering plugin to guard their registrations. The README puts it like this:

Solutions are expected to handle their own permissions, and what should or should not be displayed at any time, the Observability plugin will not add and remove items for you.

Oh, that panel padding in the loading indicator doesn't look right. I'll take a look at removing the wrapping panel.

@formgeist
Copy link
Contributor

The patch to add these menu entries is just for demonstration purposes and not part of the PR. Since the side nav can't know about the relation between specific permissions and menu entries, it's up to the registering plugin to guard their registrations.

Ah, good to know!

Oh, that panel padding in the loading indicator doesn't look right. I'll take a look at removing the wrapping panel.

Thanks!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 282 288 +6

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observability 460.3KB 461.2KB +905.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
observability 7 9 +2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observability 33.7KB 35.9KB +2.2KB
Unknown metric groups

async chunk count

id before after diff
observability 4 5 +1

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 22 20 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 261 149 -112
lens 67 45 -22
licensing 18 15 -3
lists 239 236 -3
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
securitySolution 386 342 -44
stackAlerts 101 95 -6
total -342

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @weltenwort

Copy link
Contributor

@katefarrar katefarrar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still having issues with my local environment, but I was able to look at this with @formgeist. Everything looks great. Thank you all! 👍

@weltenwort weltenwort added the auto-backport Deprecated - use backport:version if exact versions are needed label May 27, 2021
@weltenwort weltenwort merged commit 06d276e into elastic:master May 27, 2021
@weltenwort weltenwort deleted the obs-shared-navigation branch May 27, 2021 14:58
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request May 27, 2021
…tic#99380)

Co-authored-by: Kerry Gallagher <471693+Kerry350@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request May 27, 2021
…) (#100792)

Co-authored-by: Kerry Gallagher <471693+Kerry350@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com>
Co-authored-by: Kerry Gallagher <471693+Kerry350@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Project:ObservabilitySolutionNavigation release_note:enhancement Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Unified Observability Work to make the observability apps more consistent and eventually more unified as a user experience v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Observability] Implement Solution Navigation
8 participants